Skip to content

Added a feature to avoid linebreaks between option when displaying help #242

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

sybaris
Copy link

@sybaris sybaris commented Feb 12, 2018

Hi

Linked with issue #224.
I try to make this to allow users to set option in ParserSettings to have or not linebreaks between option when displaying help.

Thanks for advance

Copy link
Member

@ericnewton76 ericnewton76 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In it's current state, you actually break the interface for HelpText.AutoBuild. You need to preserve the existing method parameters with a default ParserSettings instance.

You can always mark the older method as Obsolete if it makes sense.

@ericnewton76 ericnewton76 changed the base branch from master to develop May 11, 2018 19:03
@@ -183,20 +183,20 @@ public void Dispose()
settings.EnableDashDash)(arguments, optionSpecs);
}

private static ParserResult<T> MakeParserResult<T>(ParserResult<T> parserResult, ParserSettings settings)
private /*static*/ ParserResult<T> MakeParserResult<T>(ParserResult<T> parserResult, ParserSettings settings)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to just remove the static modifier altogether. We can see the change in diffs.

{
return DisplayHelp(
parserResult,
settings.HelpWriter,
settings.MaximumDisplayWidth);
}

private static ParserResult<T> DisplayHelp<T>(ParserResult<T> parserResult, TextWriter helpWriter, int maxDisplayWidth)
private /*static*/ ParserResult<T> DisplayHelp<T>(ParserResult<T> parserResult, TextWriter helpWriter, int maxDisplayWidth)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to just remove the static modifier altogether. We can see the change in diffs.

@@ -315,7 +315,7 @@ public void Invoke_AutoBuild_for_Options_returns_appropriate_formatted_text()
});

// Exercize system
var helpText = HelpText.AutoBuild(fakeResult);
var helpText = HelpText.AutoBuild(fakeResult, new ParserSettings());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of these test updates should've alerted you to the fact that you broke the existing expected interface. This will cause problems for people simply upgrading from a patch release, we'd have to make it wait until a full version release.

As noted elsewhere, you should preserve this method and make your new method an overload with the old method calling the new method with that default ParserSettings instance.

@moh-hassan moh-hassan force-pushed the develop branch 2 times, most recently from b211712 to 746885a Compare February 3, 2020 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants